Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade to React Native v0.59.0 #741

Merged
merged 26 commits into from
Mar 15, 2019
Merged

Upgrade to React Native v0.59.0 #741

merged 26 commits into from
Mar 15, 2019

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Mar 13, 2019 •
edited
Loading

This PR tracks the effort to upgrade to the v0.59.x version of React Native.

Known issues:

  1. On the android side of things, the demo app fails to keep the title block to its original value. It seems that there is a "stray" AppContainer.setTitleAction() call that empties the title, but trying to follow the callstack in the debugger shows things that don't make sense. Looks like the JS files could be mixed up somehow. Thing is, the RN app works fine inside wpandroid so, we can probably tackle this issue separately. Seems fixed after updating with a recent develop.
  2. When using a JS Debugger in Android, the stacktrace seems like it's pointing to the wrong source files. Example:
  * Place a `debugger;` statement in the beginning of the `SplitBlock()` in `paragraph/edit.native.js`
  * Run the android demo app and enable JS debugging
  * Place the caret inside a paragraph block and hit "Enter" to split it
  * Notice the debugger pausing on a "SplitBlock" method alright (check the stacktrace panel) but the file pointed at is `image/index.js` which makes no sense.

Filed a bug report on the react-native repo: facebook/react-native#23955

@SergioEstevao SergioEstevao requested a review from etoledom March 13, 2019 09:33
@etoledom
Copy link
Contributor

First tests looks good.
Wanted to highlight that this seems to fix: #729 🎉

I'll also test integration with WPiOS. @SergioEstevao is there a branch for this?

@etoledom
Copy link
Contributor

WPiOS looks good too. I'll publish a branch if there's any.

@hypest hypest requested review from mzorz and marecar3 March 13, 2019 15:18
@marecar3
Copy link
Contributor

marecar3 commented Mar 13, 2019 •
edited
Loading

Hey @hypest, nice work here :)

  1. I found some glitch in the demo app. I have tested the application on my Nexus 5x, Android 8.0
    and when I try to tap on the Header block it automatically switches focus to the next Paragraph block.

ezgif com-video-to-gif (2)

  1. On WPAndroid there is a similar glitch when the keyboard is dismissed I want to focus the last paragraph but the focus is lost.

ezgif com-video-to-gif (3)

  1. On WPAndroid there is a bug with strikethrough button. When I select whole text in the last paragraph and bold and italic are highlighted, nothing happens after the strikethrough button is clicked.

  2. Missing padding on placeholder when a new post is created.

missing_padding

@hypest
Copy link
Contributor Author

hypest commented Mar 14, 2019

Thanks for taking this for a test drive @marecar3 !

Nice find about the focus. What I actually see is that the focus seems to move as you scroll even! This can "explain" why the focus moves when the keyboard comes up and such. Quite frustrating. Will look into it.

@etoledom
Copy link
Contributor

I couldn't reproduce any of those issues on iOS. (both example and WPiOS apps).

I do think that is necessary a new JS Bundle, since currently WPiOS is broken without metro running.

@etoledom etoledom mentioned this pull request Mar 14, 2019
Merged
@etoledom
Copy link
Contributor

Opent WPiOS PR: wordpress-mobile/WordPress-iOS#11271

@hypest
Copy link
Contributor Author

hypest commented Mar 14, 2019

There's this issue on the ReactNative repo facebook/react-native#23916 that is probably related to the scrolling/focus issue we're seeing here.

@Tug Tug mentioned this pull request Mar 14, 2019
Merged
@hypest
Copy link
Contributor Author

hypest commented Mar 14, 2019 •
edited
Loading

I pushed c2d1889 that has a workaround for the losing-focus-on-scroll issue, by disabling the Flatlist's optimization of destroying the views when they get out of the viewport. That's a real bummer of course but, we apparently will need to implement a proper support for Views recycling that is more compatible with focus preservation.

For reference, this is the PR that sets that prop to default true for Android in React Native: Rachelmorrell/react-native#170

Chances are that there are more factors at play here and this becomes an issue with v0.59.0 but, for now it's an OK workaround for us until we put in proper support for views recycling.

@hypest
Copy link
Contributor Author

hypest commented Mar 14, 2019

@marecar3 , I think the No1, No2, No3 findings you mentioned got fixed with c2d1889. Can you recheck?

Regarding finding No4 (Missing padding on placeholder), I'm not able to replicate. Tried Pixel 2XL (Android 9.0) and Nexus 5X Emulator (Android 8.0.0). Can you try again or maybe share more info about the device/OS you tried it on? Thanks!

@SergioEstevao SergioEstevao self-requested a review March 14, 2019 14:41
@hypest hypest mentioned this pull request Mar 14, 2019
Closed
@marecar3
Copy link
Contributor

@hypest nice that we fixed those, I will try the branch now and will come back with the new feedback.

@marecar3
Copy link
Contributor

marecar3 commented Mar 14, 2019 •
edited
Loading

Hey @hypest I have pointed this PR to the latest GB rnmobile/develop - 3f234faf8b811bdfd8ada5899836b3521d886163 but getting the below error :

Screen Shot 2019-03-14 at 1 30 40 PM

Not sure if it's something wrong only on my laptop

etoledom
etoledom approved these changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS side looks good and WPiOS PR is ready and waiting! 🎉

marecar3
marecar3 approved these changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @hypest I have checked, everything is working great now !!! nice one 🍾

Probably It would be good to update this branch with the latest develop and to point it to the latest Gutengerg rnmobile/develop branch so that we can have everything up to date.

@hypest
Copy link
Contributor Author

hypest commented Mar 15, 2019 •
edited
Loading

I've updated from current develop and pushed.

Fortunately, that fixed one known issue.

The Android demo app and wpandroid look good during my testing. @SergioEstevao can you do another round of smoke testing on the iOS side of things to make sure this will not disrupt anything when we merge? Thanks!

SergioEstevao
SergioEstevao approved these changes Mar 15, 2019
View reviewed changes
Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working great!

@hypest hypest merged commit 08a0962 into develop Mar 15, 2019
@hypest hypest deleted the upgrade-to-rn-059 branch March 15, 2019 14:47
@mkevins mkevins mentioned this pull request Apr 4, 2019
Closed
@hypest hypest mentioned this pull request Apr 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marecar3 marecar3 marecar3 approved these changes

@etoledom etoledom etoledom approved these changes

@SergioEstevao SergioEstevao SergioEstevao approved these changes

@mzorz mzorz Awaiting requested review from mzorz

Assignees

@SergioEstevao SergioEstevao

@hypest hypest

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hypest @etoledom @marecar3 @SergioEstevao